Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add more props to Checkbox for Donate styling #697

Merged
merged 23 commits into from
Jan 31, 2025

Conversation

michael-odonovan
Copy link
Contributor

@michael-odonovan michael-odonovan commented Jan 23, 2025

PR Titles

To pass testing pipeline, these need to follow Conventional Commits spec:

https://www.conventionalcommits.org/en/v1.0.0/
e.g.
feat: create NewForm component
or
fix: update broken link in NewForm component

PR description

What is it doing?

feat: add more props to Checkbox for Donate styling

Why is this required?

Donate styling work.

link to Jira ticket:

https://comicrelief.atlassian.net/browse/DF-191

Quick Checklist:

  • My PR title follows the Conventional Commit spec.

  • I have filled out the PR description as per the template above.

  • I have added tests to cover new or changed behaviour.

  • I have updated any relevant documentation.

Important! - lastly, make sure to squash merge...

@michael-odonovan michael-odonovan self-assigned this Jan 23, 2025
@michael-odonovan
Copy link
Contributor Author

michael-odonovan commented Jan 28, 2025

As per Slack conversation, I did try importing this branch / commit into CR.com.
It looks fine in CL.
I have only added styling props but I did simplify the way of hiding the input / make it more easy to groq.

Copy link
Contributor

@AndyEPhipps AndyEPhipps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to have borked the pre-existing bg-color styles:

Screenshot 2025-01-28 at 15 59 51

Copy link
Contributor

@AndyEPhipps AndyEPhipps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-01-31 at 11 03 13

Might need to update that border to match the new bg colour?

@michael-odonovan
Copy link
Contributor Author

michael-odonovan commented Jan 31, 2025

@AndyEPhipps
Currently on master we have red for focus state:

  :focus + span {
    border-color: ${({ theme }) => theme.color('red')};
    border-width: 1px;
  }

Is this right or should I change to something else?
ie. border is red for checked and focus.

@AndyEPhipps
Copy link
Contributor

AndyEPhipps commented Jan 31, 2025

@AndyEPhipps Currently on master we have red for focus state:

  :focus + span {
    border-color: ${({ theme }) => theme.color('red')};
    border-width: 1px;
  }

Is this right or should I change to something else? ie. border is red for checked and focus.

Ah shit, I'm sorry, my bad - I somehow missed that you'd added prop options for ALL of these colour-usages; so that green border in the example was by design 👍

Just testing a few things locally, sec

Copy link
Contributor

@AndyEPhipps AndyEPhipps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, sorry about the misunderstanding

@michael-odonovan
Copy link
Contributor Author

Nah man I was sloppy earlier sorry

@michael-odonovan michael-odonovan merged commit 2015c5c into master Jan 31, 2025
9 checks passed
@michael-odonovan michael-odonovan deleted the df_191_more_props_Checkbox_for_styling branch January 31, 2025 12:13
Copy link

🎉 This PR is included in version 8.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants